Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: ndatabase Refactor #7194

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Conversation

boskoramen
Copy link
Contributor

@boskoramen boskoramen commented Sep 22, 2024

About the pull request

Refactoring ndatabase to be a bit more readable and redefining the interface for defining entities, entity links, and views to reduce boiler plate. Using field and field type datums to validate that appropriate fields and field types are passed.

Also adding unit tests to validate views and links at the very list.

Will try to document what is going on with ndatabase since variable name changes might not be sufficient in that front.

Feedback from those more familiar with how this works is much appreciated.

TODO

Implementation

  • Update DB adapters to handle datumized fields and field types
  • Update existing DB entities to use new interfaces
  • Make foreign key for entity links connect to a field on entity
  • Make sure native functions are correctly handled
  • Extract entity management from entity meta into entity manager itself or entity managing class (separation of concerns)
  • Update internal_parse_column and internal_generate_view_query so that meta_to_load maps entity nodes to table names

Documentation

  • Add documentation for database interfaces (adapter, connection, etc)
  • Add documentation for database entities (including table definition)
  • Add documentation for views
  • Add documentation for indexes
  • Add documentation for database implementations
  • Rename abbreviated/shortened variables to be fully descriptive
  • Update the meta_to_load and meta_to_table variable names to more clearly reflect what they represent in adapters
  • Add documentation for internal_parse_column and internal_generate_view_query since aliasing is not very clear without digging into the code

Validation

  • Add unit tests to validate links are properly configured
  • Add unit tests to validate links are not duplicated
  • Add unit tests to validate view fields (especially those that are through entity links)

Misc

  • Understand DB indexing update

Nice to have's (will probably do these changes in separate PRs to avoid scope creep)

  • Interface for migrating data between tables
  • Non-entity based views
  • Interface for junction tables (many-to-many mappings)

Explain why it's good for the game

With datumized preferences port I am planning to do would like to try storing all preferences in the database instead of through savefiles. Wanted the DB to be a lot easier to work with before diving into that.

Testing Photographs and Procedure

Screenshots & Videos

tba

Changelog

no player facing changes

@cmss13-ci cmss13-ci bot added the Refactor Make the code harder to read label Sep 22, 2024
@boskoramen boskoramen changed the title Draft: ndatabase Refactor WIP: ndatabase Refactor Sep 22, 2024
@boskoramen boskoramen marked this pull request as draft September 22, 2024 08:46
@cmss13-ci cmss13-ci bot added Github We don't really know what else this belongs to Missing Changelog Maintainers always document their changes. labels Sep 23, 2024
- Also fixes failing build, although the entities as-is are not going to work
- Added a parent sql adapter class to handle the common behavior between the different adapters
- Less datum creation and entity does not care about static typing any way, can just iterate thru entity meta datum to get field types and any other field metadata
@cmss13-ci
Copy link
Contributor

cmss13-ci bot commented Oct 27, 2024

This PR has been inactive for long enough to be automatically marked as stale. This means it is at risk of being auto closed in ~ 7 days, please address any outstanding review items and ensure your PR is finished, if these are all true and you are auto-staled anyway, you need to actively ask maintainers if your PR will be merged. Once you have done any of the previous actions then you should request a maintainer remove the stale label on your PR, to reset the stale timer. If you feel no maintainer will respond in that time, you may wish to close this PR youself, while you seek maintainer comment, as you will then be able to reopen the PR yourself

@cmss13-ci cmss13-ci bot added the Stale beg a maintainer to review your PR label Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Github We don't really know what else this belongs to Missing Changelog Maintainers always document their changes. Refactor Make the code harder to read Stale beg a maintainer to review your PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant